Code Review — [codex] Extract CLI support target

Overview

This PR carves a pure KeyPathCLISupport target out of KeyPathAppKit, moving CLIVersion and printErr from Sources/KeyPathAppKit/CLI/CLIFacade.swift into a new, dependency-free module (Sources/KeyPathCLISupport/CLISupport.swift). It's a small, clean foothold extraction with 32 additions / 3 deletions and no regressions according to the PR validation.

What's Good

• Correct scope. Only the two symbols that have zero AppKit dependencies (CLIVersion, printErr) are moved. Nothing that transitively needs AppKit is dragged along.
• Import surgery is precise. Four files that previously imported KeyPathAppKit solely for these symbols now import KeyPathCLISupport instead. The two files that still need KeyPathAppKit for real reasons (CompletionsCommand.swift, ServiceStatusCommand.swift) correctly keep both imports.
• No new external dependencies. KeyPathCLISupport is Foundation-only with no package deps, which is exactly what you want for a pure support module.
• printErr implementation is correct. Writing directly to FileHandle.standardError is thread-safe and avoids the print()-to-stdout footgun.
• Documentation updated with measured data. The TEST_HYGIENE_PLAN.md additions include actual build timings, which is what future decisions should be based on.

Issues / Suggestions

1. KeyPathCLISupport is exposed as a library product — is that intentional?

// Package.swift
.library(
    name: "KeyPathCLISupport",
    targets: ["KeyPathCLISupport"]
),

Adding it to products: makes it part of the package's public surface — any consumer of this package can depend on KeyPathCLISupport by name. For a macOS app package that isn't consumed as a SwiftPM dependency externally, this is harmless. But if the intent is that this module is purely an internal implementation detail, omitting the .library product (keeping only the .target) would better signal that. Worth a conscious decision either way.

2. No dedicated test target for KeyPathCLISupport

The two symbols moved (CLIVersion.current, printErr) are simple but testable:

• CLIVersion.current returns "1.0.0" when neither app bundle is found — that fallback could silently mask version-display bugs in CLI environments where the app isn't installed (e.g. CI).
• printErr writes to stderr — straightforward to exercise.

Since the module builds in 1.33s, a KeyPathCLISupportTests target would be cheap. Not a blocker given the PR's stated "foothold" scope, but worth tracking as the follow-on work (result models / output contract types) lands.

3. Minor: CLIVersion.current fallback "1.0.0" (pre-existing, moved as-is)

return "1.0.0"  // fallback when no bundle found

This is existing behavior carried over from CLIFacade.swift. Returning a real-looking version when the app isn't installed can silently mislead scripts checking keypath --version. "unknown" or "0.0.0" would be more honest. Low priority since this is pre-existing, but the extraction is a natural opportunity to clean it up before it spreads.

Validation

The PR ran:

• swift build --target KeyPathCLISupport ✓
• swift build --product keypath-cli ✓ (still AppKit-heavy as expected)
• ./Scripts/test-lane.sh cli → 341 tests, 0 warnings, 0 errors ✓
• git diff --check ✓

Coverage looks appropriate for the change size.

Verdict

Approve with minor suggestions. The extraction is clean, the import changes are correct, and the validation is solid. The library-product question and missing tests for the new module are worth addressing — either now or as follow-on — but neither is a blocker.

Code Review

Overview

Clean, well-scoped extraction PR. It creates a pure KeyPathCLISupport target, moves CLIVersion and printErr out of KeyPathAppKit, and wires KeyPathCLI to the new module — all without breaking the existing AppKit dependency chain. The PR is honest about what it does and doesn't accomplish (it's a foothold, not full isolation), and the validation evidence in the description is solid.

What works well

• Correct access modifiers already in place. CLIVersion and printErr are both public in the original CLIFacade.swift, so the module boundary is immediately valid without a follow-up fix.
• Swift 6 concurrency mode applied to the new target — consistent with the rest of the project.
• Import cleanup is precise. Files that only needed the moved symbols (KeyPathTool.swift, Output.swift, UpdateChecker.swift) drop the KeyPathAppKit import entirely. Files that still consume AppKit symbols (CompletionsCommand.swift, ServiceStatusCommand.swift) correctly keep both imports.
• UpdateChecker.swift simplification (= { "..." }() → = "...") is valid — KeyPathConstants.Config.directory is a static constant and no lazy evaluation was needed. Good cleanup.
• TEST_HYGIENE_PLAN.md updated with timing/test evidence at both the status and milestone sections.

Suggestions

1. No test target for KeyPathCLISupport (minor)
The new module is pure Foundation with no AppKit dependency, which means it could have fast, isolated tests. The CLI lane covers it transitively, but a KeyPathCLISupportTests target would: (a) enforce the module boundary stays clean over time, and (b) provide a faster feedback loop than the 131s CLI lane. Consider adding at PR time or as a follow-up ticket.

2. CLIVersion.current returns "1.0.0" when no installed app is found (pre-existing, worth noting)
The fallback is fine for production, but any future unit tests for this module will silently get "1.0.0" rather than a real version. If tests are added, this should be injectable (e.g., a versionLookup closure default) to avoid hardcoded fallback assertions. Not a blocker for this PR since the behavior is unchanged.

3. Filename rename CLIFacade.swift → CLISupport.swift
The rename is appropriate (it's no longer a facade for AppKit), but it will break git blame continuity on these two symbols. If that matters, git log --follow handles it, but worth calling out for future spelunking.

Potential risks

None beyond the expected: KeyPathCLI still pulls in the full AppKit graph, so this doesn't reduce binary size or build time yet. The PR is explicit about this — no concern.

Verdict

Approve. The changes are correct, minimal, and consistent with the incremental extraction strategy documented in TEST_HYGIENE_PLAN.md. Adding a KeyPathCLISupportTests target (even just a smoke test) would be a worthwhile follow-up before the boundary grows.

Code Review

Overview

This PR creates a new KeyPathCLISupport target and moves CLIVersion + printErr out of KeyPathAppKit. It's a surgical, well-scoped first step in decoupling the CLI from the AppKit graph — no over-engineering, and the PR is honest that KeyPathCLI still depends on KeyPathAppKit. Scope is appropriate.

What works well

• Correctly scoped. Creates the boundary without trying to do everything at once. The PR description explicitly calls out what's left.
• Swift 6 mode. New target correctly sets .swiftLanguageMode(.v6) — consistent with the rest of the package.
• cacheFile simplification in UpdateChecker.swift. Removing the unnecessary lazy closure (the value is a compile-time constant interpolation) is a correct and clean simplification.
• Validation is thorough. Build times, test counts, zero warnings — good.
• Doc update. TEST_HYGIENE_PLAN.md is updated with the checkpoint and explicitly says to stop here for this phase. Clear stopping condition.

Issues / Suggestions

Minor: CLIVersion.current fallback silently returns "1.0.0"

return "1.0.0"  // fallback when no bundle found

This is pre-existing, but now that it lives in its own module it's more visible. If neither standard install path is found (e.g., during testing or a non-standard install), the version silently reports 1.0.0 which could mislead users or bug reports. Consider returning something like "unknown" or checking the running process bundle as a third candidate. Low urgency — the current behavior was already shipping.

Minor: No tests for the new module

The module is simple (CLIVersion + printErr), but CLIVersion.current's bundle-lookup logic and fallback are the kind of thing that can quietly break when install paths change. A targeted test (even one that asserts the fallback when no app bundle is present) would protect the extraction boundary. Not a blocker.

Observation: KeyPathCLISupport is declared as a .library product

.library(name: "KeyPathCLISupport", targets: ["KeyPathCLISupport"]),

This exposes it as a public product consumable by external packages. If it's only ever meant to be an internal module, the .library product declaration can be removed while keeping the target — this reduces public API surface. Not a correctness issue, just worth a deliberate choice.

Import changes verification

The import swaps look correct:

• KeyPathTool.swift: drops KeyPathAppKit entirely — valid since the only things pulled from AppKit were CLIVersion/printErr, both now in KeyPathCLISupport.
• CompletionsCommand.swift, ServiceStatusCommand.swift: keep both imports since they use things from both modules — correct.
• Output.swift, UpdateChecker.swift: drop KeyPathAppKit — consistent with the move.

Summary

Approve with minor notes. Clean, minimal, well-validated. The two minor issues (fallback version string, no tests for new module) are pre-existing or low-urgency. The .library product question is worth a one-line clarification but isn't a blocker. The TEST_HYGIENE_PLAN.md "stop here" note is valuable — keep it.